feat(event_handler): allow multiple CORS origins #2279
Conversation
boring-cyborg
bot
added
documentation
pull-request-size
bot
added
the
size/M
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2279 +/- ##
========================================
Coverage 97.46% 97.46%
========================================
Files 149 149
Lines 6895 6901 +6
Branches 506 509 +3
========================================
+ Hits 6720 6726 +6
Misses 137 137
Partials 38 38
☔ View full report in Codecov by Sentry. |
| ???+ tip "Multiple allowed origins?" | ||
| If you require multiple allowed origins, pass the additional origins using the `extra_origins` key. | ||
|
|
There was a problem hiding this comment.
Ruben, please add the extra_origins field in the table below.
How about adding a new tab with an example to show how to use this new field? What do you think?
There was a problem hiding this comment.
Maybe I'm dizzy in the taxi but this reads odd, perhaps what you meant was:
Multiple origins?
If you need to allow multiple origins ...
As in, Allow-Origins is an explicit CORS terminology, but here we can be more flexible in wording
| self.allow_headers = set(self._REQUIRED_HEADERS + (allow_headers or [])) | ||
| self.expose_headers = expose_headers or [] | ||
| self.max_age = max_age | ||
| self.allow_credentials = allow_credentials | ||
|
|
||
| def to_dict(self) -> Dict[str, str]: | ||
| def to_dict(self, origin: Optional[str]) -> Dict[str, str]: |
There was a problem hiding this comment.
origin: str = ""?
Annotation says it's optional but we are not setting a default value.
You can drop the Optional (None), and simply set to an empty str
There was a problem hiding this comment.
Does this mean I would have to compare if origin == "" later on instead of not origin? Doesn't it sound wrong?
There was a problem hiding this comment.
TIL not will eventually translate to __bool__, so it works for empty strings too
There was a problem hiding this comment.
I've changed my mind again and I believe it's better to keep the Optional. Reason is, the caller will try to fetch an Origin from the headers. The Origin is not always present, so mypy would complain that I can't pass an Optional to the to_dict. So I think in this case the Optional makes sense.
| ???+ tip "Multiple allowed origins?" | ||
| If you require multiple allowed origins, pass the additional origins using the `extra_origins` key. | ||
|
|
There was a problem hiding this comment.
Maybe I'm dizzy in the taxi but this reads odd, perhaps what you meant was:
Multiple origins?
If you need to allow multiple origins ...
As in, Allow-Origins is an explicit CORS terminology, but here we can be more flexible in wording
pull-request-size
bot
added
size/L
|
As promised, added E2E tests. |
There was a problem hiding this comment.
I ran the end2end test on this branch and everything is working as expected, Ruben! There are some random bugs at specific runtimes, but they are known bugs and nothing related to this change. - https://github.com/awslabs/aws-lambda-powertools-python/actions/runs/5006899451/jobs/8972788300
One more amazing work, dude!!!
leandrodamascena
changed the title
Issue number: #1006
Summary
Changes
This PR adds support for additional origins for CORS. It also changes the behavior to never return Origin information outside of a CORS request.
User experience
NOTE: this PR changes the default behavior when a non-CORS request is sent. Previously, we would return the configured Origin as part of the response. Now we only do it if there's an Origin in the request and it matches one of the allowed origins. Should we do something about this?
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.